[SPARK-16071][SQL] Checks size limit when doubling the array size in BufferHolder by clockfly · Pull Request #13829 · apache/spark

clockfly · 2016-06-21T23:57:57Z

What changes were proposed in this pull request?

This PR Checks the size limit when doubling the array size in BufferHolder to avoid integer overflow.

How was this patch tested?

Manual test.

SparkQA · 2016-06-22T01:50:36Z

Test build #60984 has finished for PR 13829 at commit 24dd723.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-06-22T01:50:36Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/BufferHolder.java

+    int bitsetWidthInBytes = UnsafeRow.calculateBitSetWidthInBytes(row.numFields());
+    if (row.numFields() > (Integer.MAX_VALUE - initialSize) / 8) {
+      throw new UnsupportedOperationException(
+        "Cannot create BufferHolder from input UnsafeRow because it is too big.");


...too big might be a bit to vague.... Can you use something like ...exceeds the maximum number of variables (268435455).

...BufferHolder from input UnsafeRow... -> ...BufferHolder for input UnsafeRow...

We only get the numFields from the unsafe row and allocate memory for it.

hvanhovell · 2016-06-22T01:52:05Z

One small comment. LGTM otherwise.

SparkQA · 2016-06-22T03:13:20Z

Test build #60993 has finished for PR 13829 at commit 6473e6d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class BufferHolderSuite extends SparkFunSuite

cloud-fan · 2016-06-22T07:01:52Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/BufferHolder.java

   * Grows the buffer by at least neededSize and points the row to the buffer.
   */
  public void grow(int neededSize) {
+    if (neededSize > Integer.MAX_VALUE / 2 - totalSize()) {


shall we move this check into the if branch below? then we can just check length * 2 <= Integer.MAX_VALUE and others can understand it very easily as there is a final byte[] tmp = new byte[length * 2]; next line.

final int length = totalSize() + neededSize;, this can cause integer overflow, as well as length * 2

cloud-fan · 2016-06-29T01:28:09Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/BufferHolder.java

  public BufferHolder(UnsafeRow row, int initialSize) {
-    this.fixedSize = UnsafeRow.calculateBitSetWidthInBytes(row.numFields()) + 8 * row.numFields();
+    int bitsetWidthInBytes = UnsafeRow.calculateBitSetWidthInBytes(row.numFields());
+    if (row.numFields() > (Integer.MAX_VALUE - initialSize - bitsetWidthInBytes) / 8) {


I don't quite understand this, we are trying to avoid overflow of this.fixedSize = UnsafeRow.calculateBitSetWidthInBytes(row.numFields()) + 8 * row.numFields(); right? Why we - initialSize here?

Check line
https://github.com/apache/spark/pull/13829/files#diff-e68c5a074209b9a20ee2aa42936571ceR55

this.buffer = new byte[fixedSize + initialSize];

SparkQA · 2016-06-29T02:26:58Z

Test build #61428 has finished for PR 13829 at commit b831e85.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-06-29T06:25:33Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/BufferHolder.java

One more thought: Can we grow the buffer to Integer.MAX_VALUE if we can't double its size? Then we have another chance to continue the execution and finish it.

@cloud-fan

Currently the limit for neededSize + totalSize is Integer.MAX_VALUE / 2, I don't see there is a big difference to enlarge the limit to Integer.MAX_VALUE.

Integer.MAX_VALUE / 2 is about 1 GB, it is quite rare for a single row to exceed this limit.

I think it's good to try our best to finish user's job instead of failing it. And it's not a lot of work, should be worth it, just grow the buffer to Integer.MAX_VALUE when neededSize + totalSize is between Integer.MAX_VALUE / 2 + 1 and Integer.MAX_VALUE

SparkQA · 2016-06-29T08:11:45Z

Test build #61452 has finished for PR 13829 at commit 336986d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-06-30T01:45:50Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/BufferHolder.java

nit: Integer.MAX_VALUE /2 -> Integer.MAX_VALUE / 2, you missed a space...

cloud-fan · 2016-06-30T01:47:55Z

LGTM except some style comments

SparkQA · 2016-06-30T03:17:21Z

Test build #61514 has finished for PR 13829 at commit 3a831e0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-06-30T03:37:02Z

Test build #61515 has finished for PR 13829 at commit 4265771.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-06-30T03:52:57Z

Test build #61517 has finished for PR 13829 at commit 943f7de.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…BufferHolder ## What changes were proposed in this pull request? This PR Checks the size limit when doubling the array size in BufferHolder to avoid integer overflow. ## How was this patch tested? Manual test. Author: Sean Zhong <seanzhong@databricks.com> Closes #13829 from clockfly/SPARK-16071_2. (cherry picked from commit 5320adc) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2016-06-30T13:58:12Z

thanks, merging to master/2.0!

tedyu · 2016-06-30T21:22:41Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/BufferHolderSuite.scala

+    var e = intercept[UnsupportedOperationException] {
+      new BufferHolder(new UnsafeRow(Int.MaxValue / 8))
+    }
+    assert(e.getMessage.contains("too many fields"))


Should this string be defined in BufferHolder and referenced here so that the test wouldn't break if the exception message is modified ?

clockfly added 2 commits June 22, 2016 07:52

fix SPARK-16071

24dd723

update UT

6473e6d

hvanhovell reviewed Jun 22, 2016
View reviewed changes

cloud-fan reviewed Jun 22, 2016
View reviewed changes

update UT

b831e85

cloud-fan reviewed Jun 29, 2016
View reviewed changes

clockfly force-pushed the SPARK-16071_2 branch 2 times, most recently from 3a831e0 to 4265771 Compare June 30, 2016 01:37

cloud-fan reviewed Jun 30, 2016
View reviewed changes

fix UT

943f7de

clockfly force-pushed the SPARK-16071_2 branch from 4265771 to 943f7de Compare June 30, 2016 01:50

asfgit closed this in 5320adc Jun 30, 2016

tedyu reviewed Jun 30, 2016
View reviewed changes

Conversation

clockfly commented Jun 21, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Jun 22, 2016

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 29, 2016

Uh oh!

cloud-fan Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 30, 2016

Uh oh!

SparkQA commented Jun 30, 2016

Uh oh!

SparkQA commented Jun 30, 2016

Uh oh!

SparkQA commented Jun 30, 2016

Uh oh!

cloud-fan commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cloud-fan Jun 29, 2016 •

edited

Loading